Repository navigation
ClickHouse: Add fuzz target comparing the tokenizer with ClickHouse's lexer - #2646
s5dsn-eqee wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2646 +/- ##
=======================================
Coverage 81.20% 81.20%
=======================================
Files 42 42
Lines 33893 33893
Branches 33893 33893
=======================================
Hits 27523 27523
Misses 2796 2796
Partials 3574 3574 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
FYI, the ClusterFuzzLite error is due to duckDB shenanigans. Hopefully will be patched soon. Will review the PR shortly. |
LucaCappelletti94
left a comment
LucaCappelletti94
left a comment
There was a problem hiding this comment.
I would suggest the objectively non trivial option to go for a crate for the ClickHouse lexer. fuzz/build.rs fails when c++ is GCC with reference to 'std' is ambiguous at Lexer.cpp:526, which breaks --all-features, and the C ABI returns token kinds as bare numbers. The other oracle targets each depend on a crate (libsqlite3-sys, pg_query, libduckdb-sys), and https://github.com/LucaCappelletti94/sqlcipher-src is an example of a crate that tracks an upstream C codebase.
Such a crate should ship clickhouse-lexer's upstream/ files unchanged, compile them with clang++, and expose a safe Lexer yielding tokens with a TokenKind enum, along with the ClickHouse commit the files come from. Every update should run clickhouse-lexer's own test suite (test/test.js) on the shipped sources before release, next to the crate's tests. A scheduled workflow, like clickhouse-lexer's sync.yml for its own releases, should open the update PR by itself whenever the lexer changes, so nobody has to remember to bump it.
The best home is ClickHouse/clickhouse-lexer itself, and you could offer the crate there on ClickHouse/clickhouse-lexer#2. That issue has had no answer since September, so publishing from your own repository as clickhouse-lexer-sys and offering to transfer it later avoids blocking on them. This PR then keeps only the target and the docs, and the lone $ check below becomes kind == TokenKind::DollarSign.
I understand it is quite a bit of a challenge, and it is a reasonable ask only in this brave new world of LLMs given every step I described is rather mechanic and fully testable & verifiable. Would you be up for it?
| cc::Build::new() | ||
| .cpp(true) |
There was a problem hiding this comment.
You should pin the compiler to clang++. When c++ is GCC (the default on Ubuntu), cargo +nightly fuzz build --features clickhouse fails with reference to 'std' is ambiguous at Lexer.cpp:526.
| cc::Build::new() | |
| .cpp(true) | |
| cc::Build::new() | |
| // LexerStandalone.h declares a `std` namespace that GCC's predeclared one makes ambiguous. | |
| .compiler("clang++") | |
| .cpp(true) |
But this is only relevant if we go for git submodules instead of a crate.
| if clickhouse_lexer_token_is_end(token) != 0 { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
You should count a lone $ as rejected. ClickHouse lexes it as DollarSign, which only its JSONPath parser consumes, so no statement holding one parses.
Furthermore, ClickHouse 26.9 rejects SELECT $$, SELECT [$], SELECT 1 AS $ and SELECT * FROM t WHERE x = $, yet the target reports SELECT $$ as a tokenizer bug.
I tested an 8-minute -fork=6 run, and 9190 of the 46504 crashes were this case. A one-byte $ token is always DollarSign, and that held for all 46504 crashes.
| if clickhouse_lexer_token_is_end(token) != 0 { | |
| return true; | |
| } | |
| if clickhouse_lexer_token_is_end(token) != 0 { | |
| return true; | |
| } | |
| // A lone `$` lexes as DollarSign, which no ClickHouse SQL statement accepts. | |
| if token_end.offset_from(token_begin) == 1 && *token_begin == b'$' as c_char { | |
| return false; | |
| } |
| fn clickhouse_lexer_token_is_end(token: c_uchar) -> c_int; | ||
| } | ||
|
|
||
| /// Whether ClickHouse's lexer reads `sql` to the end without an error token. |
There was a problem hiding this comment.
With the lone $ check, the doc comment should name it as it is hardly obvious.
| /// Whether ClickHouse's lexer reads `sql` to the end without an error token. | |
| /// Whether ClickHouse's lexer reads `sql` to the end without an error token or a lone `$`. |
3 participants
Closes: #2645
Dialects: ClickHouse